add support of OAuth 2.0/OIDC 'code with PKCE' flow (back-end) by laminelam · Pull Request #1792 · apache/solr

laminelam · 2023-07-18T20:18:00Z

https://issues.apache.org/jira/browse/SOLR-16897

This is the "back-end" part of 'code with PKCE' contribution.

The back-end code is mainly for configuration. This is where the different options are configured.

This PR adds tokenEndpoint and authorizationFlow options.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

janhoy

Looks overall good.

Please do a stab on RefGuide docs. The ref guide should encourage the use of code flow and warn that implicit flow support is deprecated.

Perhaps we can also print a WARN log in backend if implicit is in use (either explicitly or implicitly) so that users upgrading to 9.4 without reading the release notes will at least get a warn log that they should switch.

laminelam · 2023-08-17T21:04:05Z

Looks overall good.

Please do a stab on RefGuide docs. The ref guide should encourage the use of code flow and warn that implicit flow support is deprecated.

Perhaps we can also print a WARN log in backend if implicit is in use (either explicitly or implicitly) so that users upgrading to 9.4 without reading the release notes will at least get a warn log that they should switch.

Thank you @janhoy for the review.
I am planning to add a new entry to the ref guide.
BTW, I have added a WARN message.

janhoy · 2023-08-22T13:43:11Z

BTW, I have added a WARN message.

Thanks, the validation and warn log looks good 👍

Will do a final review when refguide docs are in. Also remember CHANGES.txt.

janhoy

Add refguide docs
CHANGES entry
Constant naming
Backend part of CSP header

janhoy · 2023-08-29T08:42:06Z

Eager to get this into 9.4. Let me know if we can help finalizing these two PRs

laminelam · 2023-08-29T10:44:02Z

Eager to get this into 9.4. Let me know if we can help finalizing these two PRs

@janhoy
I should push a new commit sometime today. I have changed little bit your CSP patch, I just need to add a test class for LoadAdminUiServlet.

janhoy · 2023-08-29T11:13:24Z

Eager to get this into 9.4. Let me know if we can help finalizing these two PRs

@janhoy I should push a new commit sometime today. I have changed little bit your CSP patch, I just need to add a test class for LoadAdminUiServlet.

Cool. In my patch i completely disabled CSP response for all other endpoints than the AdminUIServlet. I cannot see a reason to return that header for other endpoints. The only I can think of is if you have a solr package that exposes another UI on some path, it could be nice to protect that UI with CSP rules, so if it would be possible to have some fallback CSP header for all other requests perhaps?

laminelam · 2023-08-30T22:24:32Z

Eager to get this into 9.4. Let me know if we can help finalizing these two PRs

@janhoy I should push a new commit sometime today. I have changed little bit your CSP patch, I just need to add a test class for LoadAdminUiServlet.

Cool. In my patch i completely disabled CSP response for all other endpoints than the AdminUIServlet. I cannot see a reason to return that header for other endpoints. The only I can think of is if you have a solr package that exposes another UI on some path, it could be nice to protect that UI with CSP rules, so if it would be possible to have some fallback CSP header for all other requests perhaps?

Yes I was thinking the same thing, we don't need this header for non browser targeted servlets (API endpoint, etc.), it will be just ignored by other endpoints. Now, I am not sure of a scenario where we would need a different UI servlet exposed on a different path. Let me think about it and get back to you hopefully by tomorrow.

laminelam · 2023-08-30T22:31:53Z

Add refguide docs

CHANGES entry

Constant naming

Backend part of CSP header

Will add the CHANGES entry.

Here are the main changes to the patch:

In line with the security best practice of granting minimal necessary permissions, adjusted the registerTokenEndpointForCsp() method to incorporate the precise URL for the /token endpoint rather than a more general host:port based URL.
Thought providing the capability of passing the URLs as a comma-separated list is less confusing for the end-user/operator compared to using a prefix-based system property name. This approach also enables more concise implementation.

epugh · 2023-08-31T11:57:36Z

We do have two packages "in the wild" that come with a UI, the YASA package https://github.com/yasa-org/yasa and Splainer, https://github.com/o19s/splainer/tree/main/solr-splainer-package. Will these packages need to be updated?

If we want to make this change, and that ripples down to packages, then we should do it now before more Packages are developed ;-).

We do have a test (commented out) that validates deploying Packages that might be useful: https://github.com/apache/solr/blob/main/solr/packaging/test/test_packages.bats#L69

janhoy · 2023-08-31T14:13:09Z

We do have two packages "in the wild" that come with a UI

As stated anove, we never really needed to disable the old CSP header config in jetty.xml. Turns out the LoadAdminUiServlet simply replaces the header, which is nice.

laminelam · 2023-08-31T17:00:47Z

THANK YOU @janhoy for your extensive review and contribution.
I think the PR is now good for a merge.

janhoy

I fixed the merge conflict and CHANGES entry. Think this is good to go now. Great work!

Co-authored-by: Jan Høydahl <janhoy@apache.org>

janhoy · 2023-09-01T12:03:36Z

I'm going to force-push a squashed git-history in order for full test suite to run. Will preserve you as commit author..

…he#1792) Co-authored-by: Lamine Idjeraoui <lidjeraoui@apple.com> Co-authored-by: Jan Høydahl <janhoy@apache.org> (cherry picked from commit 931de51)

… (#1889) Co-authored-by: Lamine Idjeraoui <lidjeraoui@apple.com> Co-authored-by: Jan Høydahl <janhoy@apache.org> (cherry picked from commit 931de51)

laminelam mentioned this pull request Jul 18, 2023

SOLR-16896 Add support of OAuth 2.0/OIDC 'code with PKCE' flow (front-end) #1791

Merged

7 tasks

risdenk requested a review from janhoy July 19, 2023 13:21

risdenk approved these changes Jul 19, 2023

View reviewed changes

janhoy reviewed Aug 13, 2023

View reviewed changes

Comment thread solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTIssuerConfig.java Outdated

janhoy reviewed Aug 16, 2023

View reviewed changes

Comment thread solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTAuthPlugin.java Outdated

janhoy reviewed Aug 16, 2023

View reviewed changes

Comment thread solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTAuthPlugin.java Outdated

laminelam force-pushed the feature/SOLR-16897-oautho20-oidc-pkce-back-end branch 15 times, most recently from ee18897 to 6437c1b Compare August 17, 2023 20:27

laminelam closed this Aug 17, 2023

laminelam force-pushed the feature/SOLR-16897-oautho20-oidc-pkce-back-end branch from 6437c1b to 9cdf0e4 Compare August 17, 2023 20:28

HoustonPutman reopened this Aug 17, 2023

janhoy reviewed Aug 22, 2023

View reviewed changes

Comment thread solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTIssuerConfig.java Outdated

janhoy requested changes Aug 22, 2023

View reviewed changes

janhoy reviewed Aug 30, 2023

View reviewed changes

Comment thread solr/solr-ref-guide/modules/deployment-guide/pages/jwt-authentication-plugin.adoc Outdated

Comment thread solr/server/etc/jetty.xml Outdated

janhoy requested a review from rmuir August 31, 2023 08:20

janhoy reviewed Aug 31, 2023

View reviewed changes

Comment thread solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java Outdated

janhoy reviewed Aug 31, 2023

View reviewed changes

Comment thread solr/CHANGES.txt Outdated

Comment thread solr/CHANGES.txt Outdated

janhoy approved these changes Sep 1, 2023

View reviewed changes

SOLR-16897: Add support of OAuth 2.0/OIDC 'code with PKCE' flow

8264b15

Co-authored-by: Jan Høydahl <janhoy@apache.org>

janhoy force-pushed the feature/SOLR-16897-oautho20-oidc-pkce-back-end branch from f517f38 to 8264b15 Compare September 1, 2023 12:04

Merge branch 'main' into feature/SOLR-16897-oautho20-oidc-pkce-back-end

7ac8423

janhoy merged commit 931de51 into apache:main Sep 3, 2023

Conversation

laminelam commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laminelam commented Aug 17, 2023

Uh oh!

Uh oh!

janhoy commented Aug 22, 2023

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

janhoy commented Aug 29, 2023

Uh oh!

laminelam commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janhoy commented Aug 29, 2023

Uh oh!

Uh oh!

Uh oh!

laminelam commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laminelam commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epugh commented Aug 31, 2023

Uh oh!

janhoy commented Aug 31, 2023

Uh oh!

Uh oh!

laminelam commented Aug 31, 2023

Uh oh!

Uh oh!

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

janhoy commented Sep 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

laminelam commented Jul 18, 2023 •

edited

Loading

laminelam commented Aug 29, 2023 •

edited

Loading

laminelam commented Aug 30, 2023 •

edited

Loading

laminelam commented Aug 30, 2023 •

edited

Loading